You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I seem to recall trying to add Plotly.purge directly to destroyGraphDiv once before, and running into problems that looked like I was purging after the next test case had already started... anyway that's why I added the delay(50)().then(done), and if that's really required it doesn't seem like a good idea to add it to everydestroyGraphDiv - though perhaps we can make this into a variant just for use with gl tests? Or maybe we can detect gl contexts and only purge/delay if we find one?
A little more detail on my investigations:
Even when the tests failed in normal CI, they would consistently work if I ssh into the same CI container and run them right after they fail. But I can see the tests that fail in the normal run take a very long time in ssh.
The gl tests run much more slowly on CI than locally. The non-gl tests also run slower but only a little, whereas the gl tests run far slower on CI:
the full gl suite goes about 3x slower:
-npm run test-jasmine -- --tags=gl --skip-tags=noCI,flaky
local: ~45 sec
CI ssh ~2 min 32 sec
just parcoords is an extreme case >6x slower!
npm run test-jasmine -- --tags=gl --skip-tags=noCI,flaky parcoords:
local ~9 sec
CI ssh ~57 sec
Adding Plotly.purge doesn't make things go faster - unsurprisingly it adds a couple of seconds - but it does seem to make the timing more consistent from one test case to the next, as though the need to purge was building up and then when the system finally decided to do it on its own, it took an enormous amount of time and caused a timeout. If that's the case, then we could also consider increasing the 30-sec no activity timeout.
Even when the tests failed in normal CI, they would consistently work if I ssh into the same CI container and run them right after they fail. But I can see the tests that fail in the normal run take a very long time in ssh.
That's consistent with what I noticed in previous investigations, and the reason why I gave up debugging flaky tests on CI and added @flaky 😛
The gl tests run much more slowly on CI than locally.
Yeah, that's what you get for faking GPU processing using xvfb on a headless CI machine 😓
The parcoord tests are extremely resource intensive, that's why we originally skipped them on CI. Maybe we could try to 🔒 the same set of features with mocks with less dimensions and values?
though perhaps we can make this into a variant just for use with gl tests?
Maybe we could add a destroyGlGraphDiv method to assets/? Or perhaps we could start replacing all the Plotly.plot by Plotly.newPlot (which should clean up old gl contexts) - which will slow down our test, but should make things easier to maintain. ⚖️
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@etpinard this seems to make
parcoords_testpass reliably - and I can even take out the@flakytag I added in #2399. (I did get a gl2d failure in one of these runs though... https://circleci.com/gh/plotly/plotly.js/7227)I seem to recall trying to add
Plotly.purgedirectly todestroyGraphDivonce before, and running into problems that looked like I was purging after the next test case had already started... anyway that's why I added thedelay(50)().then(done), and if that's really required it doesn't seem like a good idea to add it to everydestroyGraphDiv- though perhaps we can make this into a variant just for use with gl tests? Or maybe we can detect gl contexts and only purge/delay if we find one?A little more detail on my investigations:
-
npm run test-jasmine -- --tags=gl --skip-tags=noCI,flakynpm run test-jasmine -- --tags=gl --skip-tags=noCI,flaky parcoords:Plotly.purgedoesn't make things go faster - unsurprisingly it adds a couple of seconds - but it does seem to make the timing more consistent from one test case to the next, as though the need to purge was building up and then when the system finally decided to do it on its own, it took an enormous amount of time and caused a timeout. If that's the case, then we could also consider increasing the 30-sec no activity timeout.